feat(symbolication): Make frame order explicit #103638
Conversation
This adds an enum `FrameOrder` with variants `callee_first` and `caller_first` to distinguish two common ways stack frames are stored, processed, and displayed (innermost frame at the top and at the bottom, respectively). This enum is now passed to all stacktrace-processing methods Symbolicator methods. Since Symbolicator does not (yet) read the parameter, there are no functional changes. The intention of this is to clarify in which order frames are sent to Symbolicator, which currently differs between platforms because we never properly documented or enforced this. Once Symbolicator becomes able to process the new parameter, there may also be opportunities for simplification. As a somewhat related change, this also adjusts some JS tests so they sort the "scraping_attempts" returned by Symbolicator before checking them. This is sound because there is no intended order of scraping attempts anyway, and makes sure tests pass once Symbolicator is allowed to process frames in a different order.
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@viglia I updated the order for profiling. Please feel free to suggest any expansions to the comments that may clarify the logic. |
| ], | ||
| # Methods in a profile aren't inherently ordered, but the order of returned | ||
| # inlinees should be caller first. | ||
| frame_order=FrameOrder.caller_first, |
There was a problem hiding this comment.
Bug: Inconsistent frame order between symbolication contexts
The frame order passed to Symbolicator differs inconsistently between profile symbolication and Android deobfuscation. Profile symbolication uses FrameOrder.callee_first (line 434) with comment stating "returned inlinees should be ordered callee first", while Android deobfuscation uses FrameOrder.caller_first (line 962) with comment stating "the order of returned inlinees should be caller first". Both code paths call symbolicator.process_jvm() with different frame_order values, and the PR discussion indicates these methods shouldn't have different orders since they're sending complete method sets rather than individual stacks. The comments contradict each other, suggesting misalignment in intent.
This adds an enum
FrameOrderwith variantscallee_firstandcaller_firstto distinguish two common ways stack frames are stored, processed, and displayed (innermost frame at the top and at the bottom, respectively). This enum is now passed to all stacktrace-processing methods Symbolicator methods. Since Symbolicator does not (yet) read the parameter, there are no functional changes.The intention of this is to clarify in which order frames are sent to Symbolicator, which currently differs between platforms because we never properly documented or enforced this. Once Symbolicator becomes able to process the new parameter, there may also be opportunities for simplification.
As a somewhat related change, this also adjusts some JS tests so they sort the "scraping_attempts" returned by Symbolicator before checking them. This is sound because there is no intended order of scraping attempts anyway, and makes sure tests pass once Symbolicator is allowed to process frames in a different order.
Fixes INGEST-640.